Implement operation hooks for EmbedsMany methods#911
Conversation
|
Nice 👍 Thanks Miroslav for keeping me in the loop :-) I went through the logic and it seems although you have added some tests in Also I liked your suggestion here for breaking Thanks :-) |
|
@Amir-61 thank you for reviewing the code.
Ah, good catch, makes me wonder how could I miss this 8-O. Anyways, I added a fixup commit a1f56e6 which adds missing implementation and fixes another test failure.
I agree we should eventually split However, I want to keep this pull request small and focused, I don't want to move code around here. In my comment, I was proposing to apply the new approach only for new code. That's what I did here too: new tests for operation hooks are following the new convention, but the old tests in Now I don't mind refactoring the old code too, we should eventually do it, but right now I feel there are more important things to address compared to this refactoring. |
|
LGTM |
|
LGTM as well... I just wonder why 12 tests pass out of 49 tests. Added: I re-run tests |
|
@slnode test please |
create() triggers
- before save
- after save
updateById() triggers
- before save
- after save
destroy() triggers
- before delete
- after delete
The implementation here is intentionally left with less features
than the regular DAO methods provide, the goal is to get a partial
(but still useful!) version released soon.
Limitations:
- `before save` & `after save` hooks don't provide `ctx.isNewInstance`
- async validations are not supported yet
- `persist` and `loaded` hooks are not triggered at all
- `before delete` hook does not provide `ctx.where` property and
it's not possible to change the outcome of `destroy()` using this
hook. Note that regular DAO does support this.
- updating embedded instances triggers update of the parent (owning)
model, which is correct and expected. However, the context provided
by `before save` and `after save` hooks on the parent model is sort of
arbitrary and may include wrong/extra data. The same probably applies
to the scenario when deleting embedded instances triggers update of
the parent model.
a1f56e6 to
9bde8f8
Compare
create() triggers
- before save
- after save
updateById() triggers
- before save
- after save
destroy() triggers
- before delete
- after delete
The implementation here is intentionally left with less features
than the regular DAO methods provide, the goal is to get a partial
(but still useful!) version released soon.
Limitations:
- `before save` & `after save` hooks don't provide `ctx.isNewInstance`
- async validations are not supported yet
- `persist` and `loaded` hooks are not triggered at all
- `before delete` hook does not provide `ctx.where` property and
it's not possible to change the outcome of `destroy()` using this
hook. Note that regular DAO does support this.
- updating embedded instances triggers update of the parent (owning)
model, which is correct and expected. However, the context provided
by `before save` and `after save` hooks on the parent model is sort of
arbitrary and may include wrong/extra data. The same probably applies
to the scenario when deleting embedded instances triggers update of
the parent model.
[back-port of #911]
create() triggers
- before save
- after save
updateById() triggers
- before save
- after save
destroy() triggers
- before delete
- after delete
The implementation here is intentionally left with less features
than the regular DAO methods provide, the goal is to get a partial
(but still useful!) version released soon.
Limitations:
- `before save` & `after save` hooks don't provide `ctx.isNewInstance`
- async validations are not supported yet
- `persist` and `loaded` hooks are not triggered at all
- `before delete` hook does not provide `ctx.where` property and
it's not possible to change the outcome of `destroy()` using this
hook. Note that regular DAO does support this.
- updating embedded instances triggers update of the parent (owning)
model, which is correct and expected. However, the context provided
by `before save` and `after save` hooks on the parent model is sort of
arbitrary and may include wrong/extra data. The same probably applies
to the scenario when deleting embedded instances triggers update of
the parent model.
[back-port of loopbackio#911]
|
Are you planning on providing the isNewInstance property for the 'before save' and 'after save' methods in the future? |
4 participants
create() triggers
updateById() triggers
destroy() triggers
The implementation here is intentionally left with less features
than the regular DAO methods provide, the goal is to get a partial
(but still useful!) version released soon.
Limitations:
before save&after savehooks don't providectx.isNewInstancepersistandloadedhooks are not triggered at allbefore deletehook does not providectx.whereproperty andit's not possible to change the outcome of
destroy()using thishook. Note that regular DAO does support this.
model, which is correct and expected. However, the context provided
by
before saveandafter savehooks on the parent model is sort ofarbitrary and may include wrong/extra data. The same probably applies
to the scenario when deleting embedded instances triggers update of
the parent model.
Connect to #497
See the original implementation proposed by @fabien in #516
/to @raymondfeng @Amir-61 please review